Skip to content

Conversation

@nytian
Copy link
Contributor

@nytian nytian commented Apr 22, 2025

This PR adds a new property IDictionay<string, object?> Properties at TaskOrchestrationContext, which allows DurableTaskOptions from WebJobs.Extensions.DurableTask can be passed through Microsoft.DurableTask.Protobuf.OrchestratorRequest.

Related PR:

@nytian nytian changed the title Add Properties to TaskOrchestrationContext Add New Property Properties to TaskOrchestrationContext Apr 22, 2025
@nytian nytian marked this pull request as ready for review April 22, 2025 17:45
@nytian nytian requested a review from jviau April 24, 2025 20:41
throw new ArgumentNullException(nameof(properties));
}

this.Properties = properties.ToDictionary(pair => pair.Key, pair => pair.Value);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Converting from IEnumerable<KVP> to a dictionary is dangerous because IEnumerable<KVP> allows duplicate keys whereas dictionary does not. We should change the properties parameter to be a dictionary instead of IEnumerable<KVP> to avoid these kinds of problems.

Copy link
Member

@jviau jviau Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is safe if you use a foreach loop and set the key each time, so last key wins. My preference for IEnumerable is only to follow the "take in least derived, return most derived" principal. But I am not unmoving in that. If you disagree and want IDictionary, that is fine.

Copy link
Contributor Author

@nytian nytian Apr 25, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Chris and Jacob. I updated this to be Dictionary. Because to write a foreach loop, I have to either grant Properties private set, or adding a new field to store this and then pass the value to Properties. Just feel like using Dictionary directly here seems like a more straightforward and cleaner way in this case. Let me know if you prefer others.

Copy link
Member

@jviau jviau Apr 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am okay with going back to dictionary, but I don't see why you need a private set. The following is valid without any setter on Properties:

this.Properties = new();
foreach ((string key, object? value) in properties)
{
    this.Properties[key] = value;
}

or if this.Properties is IReadOnlyDictionary<string, object?>, then you can do this:

Dictionary<string, object?> props = new();
foreach ((string key, object? value) in properties)
{
    props[key] = value;
}

this.Properties = props;

@nytian nytian requested a review from cgillum April 25, 2025 21:07
@nytian nytian requested a review from jviau April 29, 2025 19:49
Copy link
Member

@cgillum cgillum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple of minor nits

@nytian nytian merged commit 78a46df into main May 2, 2025
4 checks passed
@nytian nytian deleted the nytian/context-add-config branch May 2, 2025 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants